New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace unknown remote function #776

Merged

scohen merged 7 commits into elixir-lsp:master from Qarma-inspect:replace-unknown-function

Mar 23, 2023

Contributor

sheldak commented Nov 23, 2022 •

edited

Loading

I've prepared that PR to propose extending code actions by replacing unknown remote functions. For example, the following code:

defmodule Example do
  def foo do
    var = Enum.counts([2, 3])
  end
end

contains an unknown function: counts/1. After this change, we would get 2 possible Quick Fixes:

Replace unknown function with 'Enum.concat'
Replace unknown function with 'Enum.count'

that rewrite our code to:

defmodule Example do
  def foo do
    var = Enum.concat([2, 3])
  end
end

or

defmodule Example do
  def foo do
    var = Enum.count([2, 3])
  end
end

respectively.

In our example, Quick Fixes were generated from the following Elixir warning:

Enum.counts/1 is undefined or private. Did you mean:

      * concat/1
      * concat/2
      * count/1
      * count/2

I find such a feature very convenient, so I'm waiting for comments and feedback.

sheldak changed the title ~~Replace unknown function~~ Replace unknown remote function

Collaborator

lukaszsamson commented Jan 1, 2023

Nice work @sheldak . The problem with that approach is that it's error prone. We recently had to revert a similar text based code transformation (#775). A better option would be to express the code changes in terms of AST transformations. See #773

Contributor Author

sheldak commented Jan 4, 2023

@lukaszsamson thanks for the update! So I will wait with contributing until #773 is merged.

Contributor

scohen commented Jan 23, 2023

@sheldak #773 has landed.

Contributor Author

sheldak commented Jan 24, 2023

@scohen That's great! I will definitely come back to this feature soon


          add replacing unknown remote function to code actions

e01f381

sheldak force-pushed the replace-unknown-function branch from 7e9109e to e01f381 Compare

March 9, 2023 11:13

sheldak added 2 commits

March 9, 2023 12:17


          Merge branch 'elixir-lsp:master' into replace-unknown-function

4e11184


          fix aliases

df73277

Contributor Author

sheldak commented Mar 9, 2023

@scohen I've updated that so now it should fit the new way of doing code actions

scohen requested changes

View reviewed changes

apps/language_server/lib/language_server/experimental/code_mod/replace_remote_function.ex Outdated

Comment on lines 19 to 20

		{:., meta1, [{:__aliases__, meta2, ^module}, ^name]} ->
		{:., meta1, [{:__aliases__, meta2, module}, suggestion]}

Contributor

scohen Mar 9, 2023

nit: meta1 and meta2 are not descriptive names, one is the function call meta and the other is the module meta. Suggest function_meta and module_meta

apps/language_server/lib/language_server/experimental/code_mod/replace_remote_function.ex Outdated Show resolved Hide resolved

apps/language_server/lib/language_server/experimental/provider/handlers/code_action.ex Outdated Show resolved Hide resolved

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated

+                @pattern ~r/(.*)\/(.*) is undefined or private. .*:\n(.*)/s
+                @spec pattern() :: Regex.t()
+                def pattern, do: @pattern

Contributor

scohen Mar 9, 2023

this exposes private implementation details, I have an example of how to handle this without doing so below.

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated

+                    |> Enum.map(&String.to_atom/1)
+                    |> List.pop_at(-1)
+                  {module, name}

Contributor

scohen Mar 9, 2023

nit, module isn't actually a module, it's a list, and it will be missing the leading Elixir that all elixir modules have.

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated Show resolved Hide resolved

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated Show resolved Hide resolved

...uage_server/lib/language_server/experimental/provider/code_action/replace_with_underscore.ex Outdated

-                def apply(%CodeAction{} = code_action) do
-                  source_file = code_action.source_file
-                  diagnostics = get_in(code_action, [:context, :diagnostics]) || []
+                @pattern ~r/variable "([^"]+)" is unused/

Contributor

scohen Mar 9, 2023

nit: Pattern is a much less descriptive name than variable_re, but we should not be exposing this anyways.

apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs Outdated Show resolved Hide resolved

apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs Outdated Show resolved Hide resolved

scohen reviewed

View reviewed changes

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated Show resolved Hide resolved

scohen reviewed

View reviewed changes

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated Show resolved Hide resolved

scohen requested changes

View reviewed changes

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated

+                def apply(source_file, diagnostic) do
+                  with {:ok, module, name} <- extract_function(diagnostic.message),
+                       {:ok, suggestions} <- extract_suggestions(diagnostic.message),
+                       one_based_line = extract_line(diagnostic),

Contributor

scohen Mar 15, 2023

Actually, I'm matching against the Types.Diagnostic module in the extract_line
It's better to do it in apply, you wrote a typespec, so why not:

def apply(%SourceFile{} = source_file, %Diagnostic{} = diagnostic) do 
...
end

it seems like none of the keys in diagnostic.range.start.line have an optional type, so why isn't it guaranteed that one_based_line is an integer

Because elixir is a dynamic language and there are no types.

What I'm saying is: move one_based_line out of the with statement.

...uage_server/lib/language_server/experimental/provider/code_action/replace_remote_function.ex Outdated

Comment on lines 137 to 143

+                    module_list
+                    |> Enum.map(fn module ->
+                      module
+                      |> Atom.to_string()
+                      |> String.trim_leading("Elixir.")
+                    end)
+                    |> Enum.join(".")

Contributor

scohen Mar 15, 2023

then this is just

module_string = Enum.map_join(module_list, ".", &Atom.to_string/1)

apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs Outdated Show resolved Hide resolved

apps/language_server/test/experimental/provider/code_action/replace_with_underscore_test.exs Outdated Show resolved Hide resolved

sheldak added 2 commits

March 16, 2023 14:09


          adjust to review

3b5d778


          fix replacing for aliased modules

9a01a18

Contributor Author

sheldak commented Mar 16, 2023

I've found out that replacing wasn't working if the module of the function to replace was aliased. Fixed in the second commit (9a01a18).

sheldak requested a review from scohen

March 16, 2023 15:41

scohen reviewed

View reviewed changes

Contributor

scohen left a comment

One more comment regarding the tests, and we're good to merge

apps/language_server/test/experimental/provider/code_action/replace_remote_function_test.exs Outdated Show resolved Hide resolved


          update tests

90cb566

sheldak requested a review from scohen

March 23, 2023 12:22

scohen reviewed

View reviewed changes

apps/language_server/test/experimental/provider/code_action/replace_remote_function_test.exs

+                        Enum.concat(a)
+                      end
+                    end
+                  ] |> Document.new()

Contributor

scohen Mar 23, 2023

wow, this works very well; I think the structural == will even work with documents. Very nice.

scohen approved these changes

View reviewed changes


          format code

b92e07a

scohen merged commit 6f2f071 into elixir-lsp:master

sheldak deleted the replace-unknown-function branch

April 5, 2023 08:21

sheldak mentioned this pull request

Add replacing unknown remote function to code actions lexical-lsp/lexical#443

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet